Fix pinyin mode persisting after switching to another layout - #878
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request simplifies the setKeyboardLayout method in Mandarin.h by directly assigning the boolean result of the layout comparison to pinyin_mode_. The reviewer identified a potential state inconsistency when switching to the Hanyu Pinyin layout, where pinyin_sequence_ is cleared but syllable_ is not, and suggested populating pinyin_sequence_ from the existing syllable_ to maintain consistency.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| void setKeyboardLayout(const BopomofoKeyboardLayout* layout) { | ||
| layout_ = layout; | ||
| pinyin_mode_ = layout == BopomofoKeyboardLayout::HanyuPinyinLayout(); |
There was a problem hiding this comment.
When switching from a standard keyboard layout to the Hanyu Pinyin layout, pinyin_mode_ is set to true and pinyin_sequence_ is cleared to "". However, syllable_ is not cleared. This creates an inconsistent state where isEmpty() returns false (since syllable_ is not empty) but composedString() returns "" (since it returns pinyin_sequence_ when pinyin_mode_ is true).
Furthermore, any subsequent keypress or backspace will completely overwrite or clear the existing syllable without preserving the composition.
To maintain consistency and preserve the composition (similar to how it is preserved when switching from Hanyu Pinyin to a standard layout), pinyin_sequence_ should be populated from the existing syllable_ using syllable_.HanyuPinyinString(true, true).
Here is the recommended implementation:
void setKeyboardLayout(const BopomofoKeyboardLayout* layout) {
layout_ = layout;
pinyin_mode_ = layout == BopomofoKeyboardLayout::HanyuPinyinLayout();
if (pinyin_mode_) {
pinyin_sequence_ = syllable_.HanyuPinyinString(true, true);
}
} void setKeyboardLayout(const BopomofoKeyboardLayout* layout) {
layout_ = layout;
pinyin_mode_ = layout == BopomofoKeyboardLayout::HanyuPinyinLayout();
if (pinyin_mode_) {
pinyin_sequence_ = syllable_.HanyuPinyinString(true, true);
}
}There was a problem hiding this comment.
The HanyuPinyinString section doesn’t make any sense to me...
|
我有點受不了 XIB ，打算用 Swift UI 重寫 preference 了…。 |
lukhnos
left a comment
lukhnos
left a comment
There was a problem hiding this comment.
LGTM. Thanks for catching this bug!
This picks up the changes from: - openvanilla/McBopomofo#878 - openvanilla/McBopomofo#883 - openvanilla/McBopomofo#886
3 participants
This PR fixes an issue where switching from the Hanyu Pinyin keyboard layout to another layout would leave the input method in Pinyin mode.
pinyin.mp4
On an unrelated note, the labels and controls in the Preferences window are pretty badly misaligned, and I suck at laying things out in XIB 🥲 If anyone’s willing to help, that’d be awesome! Or maybe it’s finally time to migrate to SwiftUI XDD